Skip to content

fix(ts): tsc TS2322 in audit-agencysignature-main-tip — narrow ArgStep.key#887

Merged
AceHack merged 2 commits intomainfrom
fix/tsc-audit-agencysignature-mode-key
Apr 30, 2026
Merged

fix(ts): tsc TS2322 in audit-agencysignature-main-tip — narrow ArgStep.key#887
AceHack merged 2 commits intomainfrom
fix/tsc-audit-agencysignature-mode-key

Conversation

@AceHack
Copy link
Copy Markdown
Member

@AceHack AceHack commented Apr 30, 2026

Summary

Fix a strict-typecheck error in tools/hygiene/audit-agencysignature-main-tip.ts that was introduced when slice 9 (#882) ported the bash original.

The error

tools/hygiene/audit-agencysignature-main-tip.ts(124,5): error TS2322:
  Type 'string' is not assignable to type '"head" | "commit" | "max" | "since"'.

Why it's a bug

MutableArgs.mode is Mode = "head" | "commit" | "max" | "since"; all other fields are string. The ArgStep "ok" variant declared key: keyof MutableArgs which includes "mode", but step.value is always derived from argv (so always a string). The line args[step.key] = step.value (124) therefore failed assignability narrowing.

In practice the assignment is never executed for the "mode" case — the mode is set via the parallel setMode field, never through key/value. The fix is purely a type narrowing: introduce StringArgKey = "commitSha" | "maxN" | "sinceDate" | "branch" | "v1ShipDate" and use that for the key field. No runtime change.

Why CI didn't catch it

gate.yml runs dotnet build for F#/C# but no tsc --noEmit step for TS scripts. ESLint (typed-linting) catches many type issues but not this specific assignability narrowing. Adding a tsc --noEmit step to gate.yml is queued as a separate follow-up.

Test plan

  • bun --bun tsc --noEmit -p tsconfig.json is clean (was 1 error, now 0).
  • npx eslint tools/hygiene/audit-agencysignature-main-tip.ts is clean.
  • Smoke-test --help exits 0 with the expected usage block.
  • Smoke-test --commit HEAD produces an identical AgencySignature v1 main-tip audit block (verified locally — same fields, same values).
  • CI gate.

🤖 Generated with Claude Code

…p.key

`MutableArgs.mode` is the literal union `Mode = "head" | "commit" | "max" | "since"`;
all other fields are `string`. The ArgStep "ok" variant declared `key: keyof MutableArgs`
which includes `"mode"`, but `step.value` is always a string from argv. The assignment
`args[step.key] = step.value` (line 124) therefore failed strict typecheck:

    error TS2322: Type 'string' is not assignable to type
                  '"head" | "commit" | "max" | "since"'.

The mode is in fact set via the parallel `setMode` field, never via key/value.
Narrow `key` to a new `StringArgKey` literal union covering only the string-typed
fields. No runtime change — the assignment was already correct in practice; only
the type was too wide.

Found via `bun --bun tsc --noEmit -p tsconfig.json`. CI does not currently run
tsc on TS scripts (gate.yml runs `dotnet build` for F#/C#; eslint catches typed-
linting issues but missed this specific assignability narrowing). Adding a
`tsc --noEmit` step to the gate workflow is queued as a separate follow-up so
this class of error fails CI in the future.
Copilot AI review requested due to automatic review settings April 30, 2026 04:51
@AceHack AceHack enabled auto-merge (squash) April 30, 2026 04:51
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a strict TypeScript type error in the TS/Bun port of the audit-agencysignature-main-tip hygiene script by narrowing the arg-parsing assignment key to only string-valued fields, aligning the ArgStep type with actual runtime behavior.

Changes:

  • Introduced StringArgKey to represent only the string-valued keys of MutableArgs.
  • Updated ArgStep and requiresNext typing to use StringArgKey, resolving TS2322 at the args[step.key] = step.value assignment.

Comment thread tools/hygiene/audit-agencysignature-main-tip.ts Outdated
AceHack added a commit that referenced this pull request Apr 30, 2026
…888)

Captures the CI gap surfaced during slice-9 #882 post-merge audit
on 2026-04-30: a real TS2322 strict-typecheck error shipped to main
because gate.yml runs `dotnet build` for F#/C# but no `tsc --noEmit`
for TypeScript scripts under `tools/**`. ESLint with typed-linting
catches most issues but not all assignability narrowings.

The specific bug fix shipped as PR #887. This row tracks the
*missing gate* — adding `lint (tsc tools)` to gate.yml is S-effort
follow-up modeled after `lint-shell` / `lint-markdown`.

Composes with B-0086 (TS+Bun migration). The script surface under
`tools/` is at 32 ported files and growing — without the tsc gate,
this class of error will recur silently.
Copilot caught maintainability drift in the original PR: the
hand-listed `StringArgKey = "commitSha" | "maxN" | ...` union
duplicates the string-valued field names of `MutableArgs`. If
`MutableArgs` gains a new field, the union silently goes stale.

Reframed as `Exclude<keyof MutableArgs, "mode">` so the type
relationship is enforced by the compiler. Required moving the
`MutableArgs` definition above `StringArgKey` (compile-order
constraint).

No runtime change. tsc + eslint both clean.
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@AceHack AceHack merged commit 5dc1abd into main Apr 30, 2026
23 checks passed
@AceHack AceHack deleted the fix/tsc-audit-agencysignature-mode-key branch April 30, 2026 05:01
AceHack added a commit that referenced this pull request Apr 30, 2026
…on (#890)

Closes the CI gap surfaced during slice-9 #882 post-merge audit.
ESLint with typed-linting catches most TS issues but not all
assignability narrowings; PR #887 documented the specific bug class
(TS2322 in audit-agencysignature-main-tip.ts:124 shipped to main
because gate.yml had `dotnet build` for F#/C# but no tsc step for
tools/**/*.ts).

Implementation modeled after lint-shell + lint-markdown:
1. Cache install.sh outputs (mise + bun + dotnet tools).
2. install.sh with 5-attempt retry/backoff for transient CDN 502s.
3. `bun install --frozen-lockfile` to materialize node_modules with
   typescript@6.0.3 + eslint stack from package.json.
4. `bun --bun tsc --noEmit -p tsconfig.json` — strict typecheck.

Pinned to ubuntu-24.04 (OS-independent typecheck), 5-min timeout
(matches lint-markdown's runtime budget).

Same shape as round-30 semgrep elevation: codified rules
(tsconfig strict mode) without a gate aren't a control.

Updates B-0106 status from open → in-progress.
AceHack added a commit that referenced this pull request Apr 30, 2026
…on, bash shell, file-read errors

Five real findings from Copilot+Codex on PR #898:

Copilot P1 — Exit codes 0|2|64 per repo-scripting.md:
  Switched return 1 → return 64 for invocation/usage errors
  (--model/--file/--context-cmd missing values, unknown flag,
  empty prompt). Aligns with the conventions in docs/best-
  practices/repo-scripting.md §exit-codes. Process-related
  errors (gemini missing on PATH, build-prompt error) keep 1
  for tooling/input failure.

Copilot P1 — spawnSync result.status ?? 1 collapses launch failures:
  Added classifySpawnFailure helper (4-case: status set /
  ENOENT → 127 / signal / other) — same pattern from PR #887.
  Distinguishes ENOENT/permission/signal from a normal non-zero
  exit, with a contextual stderr message.

Codex P2 — eval vs /bin/sh syntax difference:
  Switched /bin/sh -c → /bin/bash -c so the bash original's eval
  semantics (accepting bash-only features like `[[ ]]`, brace
  expansion, process substitution) are preserved. /bin/sh on
  Ubuntu is dash and rejects these.

Codex P2 — file-read failures silently dropping context:
  readHead() now returns ReadHeadResult { ok, content, error }.
  buildFullPrompt() propagates the error to the user via stderr
  instead of pretending context was attached when read failed
  (permission denied, race, etc.).

Threads on already-addressed findings (PRRT_kwDOSF9kNM5-pd16
runContextCmd full-buffering + PRRT_kwDOSF9kNM5-pd2a readHead
full-file-read) were resolved in the parse-error-fix commit
ceed1e7.

Threads on PRRT_kwDOSF9kNM5-pd2U PREAMBLE-names: pushing back
because the PREAMBLE is prompt content sent verbatim to the
LLM peer, not pure code-surface; changing it diverges from
the bash original byte-equivalence. A separate task to retool
PREAMBLE attribution on both bash AND TS together is the
right shape, not a one-side rename here.
AceHack added a commit that referenced this pull request Apr 30, 2026
…igration (#898)

* ts(B-0086): port 1 peer-call sibling (.sh→.ts) — slice 16 of TS/Bun migration

* ts(slice-16, wip 1/N): port peer-call/gemini (.sh→.ts)

Sibling of slice-15 grok port (PR #896). Wraps 'gemini -p'
non-interactive headless mode. Read-only safety preserved
(--approval-mode plan + --skip-trust per the Copilot fix on
the bash original PR #28).

Reuses the structural pattern from grok.ts:
- classifyFlag + MutableArgState parser (under cog-complexity 15)
- spawnSync + stdio:inherit for live LLM streaming
- isRegularFile + readHead helpers
- /bin/sh -c for context-cmd (same trust as bash eval)
- PREAMBLE preserved verbatim with Gemini-specific framing
  (proposes role per four-ferry consensus)

Lint-clean: bun --bun tsc --noEmit + eslint strictTypeChecked +
sonarjs all pass. Argument-validation byte-equivalent.

Note: the same CodeQL js/indirect-command-line-injection alert
will fire here as on grok.ts; per B-0107 (filed in PR #897)
the per-PR dismissal pattern applies. Will dismiss after PR opens.

* review(slice-16): address Codex P1 + P2 — head-only file read + pipe-through-head context-cmd

Two real Codex findings on PR #898 (both also apply to grok.ts —
backport in follow-up PR):

P1 — Read only file head instead of loading full file:
  Original used readFileSync + Buffer.subarray which loads the
  entire file into memory then slices. For large artifacts (logs,
  dumps) this is a memory-spike regression vs bash 'head -c 20000'.
  Replaced with openSync + readSync(fd, buf, 0, bytes, 0) which
  reads only the first FILE_HEAD_BYTES bytes from disk. Wrapped
  in try/finally for fd cleanup.

P2 — Truncate context command output at source:
  Original used spawnSync with maxBuffer up to 64MB then sliced
  to CTX_HEAD_BYTES afterward. For high-volume commands (wide
  'git diff' ranges) this blocks longer and uses more memory
  than the bash original's '... | head -c 20000' pipeline that
  short-circuits at the boundary.
  Reframed wrapped command as: (cmd) 2>&1 | head -c <N>
  so the shell pipeline truncates at source. Same trust contract
  preserved (user supplies cmd; we just augment with head-c).

Verified locally: 100MB test file reads only 20000 bytes via the
new head-only path.

* review(slice-16): preserve shell parse errors per Codex P2 + Copilot (#899 finding backport)

Same fix as #899 backport — runContextCmd was returning only
result.stdout, dropping shell-process stderr (where parse errors
land, OUTSIDE the (cmd) 2>&1 redirect). Concat stdout+stderr then
slice to CTX_HEAD_BYTES.

* review(slice-16): address #898 P1+P2 — exit codes, spawn classification, bash shell, file-read errors

Five real findings from Copilot+Codex on PR #898:

Copilot P1 — Exit codes 0|2|64 per repo-scripting.md:
  Switched return 1 → return 64 for invocation/usage errors
  (--model/--file/--context-cmd missing values, unknown flag,
  empty prompt). Aligns with the conventions in docs/best-
  practices/repo-scripting.md §exit-codes. Process-related
  errors (gemini missing on PATH, build-prompt error) keep 1
  for tooling/input failure.

Copilot P1 — spawnSync result.status ?? 1 collapses launch failures:
  Added classifySpawnFailure helper (4-case: status set /
  ENOENT → 127 / signal / other) — same pattern from PR #887.
  Distinguishes ENOENT/permission/signal from a normal non-zero
  exit, with a contextual stderr message.

Codex P2 — eval vs /bin/sh syntax difference:
  Switched /bin/sh -c → /bin/bash -c so the bash original's eval
  semantics (accepting bash-only features like `[[ ]]`, brace
  expansion, process substitution) are preserved. /bin/sh on
  Ubuntu is dash and rejects these.

Codex P2 — file-read failures silently dropping context:
  readHead() now returns ReadHeadResult { ok, content, error }.
  buildFullPrompt() propagates the error to the user via stderr
  instead of pretending context was attached when read failed
  (permission denied, race, etc.).

Threads on already-addressed findings (PRRT_kwDOSF9kNM5-pd16
runContextCmd full-buffering + PRRT_kwDOSF9kNM5-pd2a readHead
full-file-read) were resolved in the parse-error-fix commit
ceed1e7.

Threads on PRRT_kwDOSF9kNM5-pd2U PREAMBLE-names: pushing back
because the PREAMBLE is prompt content sent verbatim to the
LLM peer, not pure code-surface; changing it diverges from
the bash original byte-equivalence. A separate task to retool
PREAMBLE attribution on both bash AND TS together is the
right shape, not a one-side rename here.

* review(slice-16) round-2: revert exit-code 64 → 1; fix commandAvailable

Three new findings on the latest push:

Codex P2 (PRRT_kwDOSF9kNM5-pon2) + Copilot P0 x2 — exit codes:
  Last commit switched to exit 64 per docs/best-practices/repo-
  scripting.md. Reviewers correctly point out tools/peer-call/
  README.md is the more specific spec and says 0/1/2 uniform
  across all three peer-call wrappers. Specific wins on overlap.
  Reverted ArgError.exitCode 64 → 1 + the empty-prompt return
  64 → 1. Matches grok.ts (slice 15) and the bash original.

Copilot P1 — commandAvailable() shape:
  Was using `<cmd> --version` and requiring exit 0. Some CLIs
  exit non-zero on --version. Bash uses `command -v <cmd>`
  (PATH existence check, no execution). Switched to
  `spawnSync('/bin/sh', ['-c', \`command -v "${cmd}"\`])`
  to match bash semantics.

Copilot P1 (PRRT_kwDOSF9kNM5-ppuR) — PREAMBLE names: same as
prior thread; pushing back with same rationale (PREAMBLE is
verbatim prompt content sent to LLM, not pure code-surface;
preserving bash-equivalence). Will reply on the new thread
the same way.

Copilot P2 (PRRT_kwDOSF9kNM5-ppt6) — PR description says /bin/sh
but code uses /bin/bash. The /bin/bash switch was a Codex P2 from
the prior round (preserve eval semantics for bash-only features).
Will update PR description, not the code.
AceHack added a commit that referenced this pull request Apr 30, 2026
…igration (#900)

* ts(slice-17, wip 1/N): port peer-call/codex (.sh→.ts)

Third sibling of the peer-call cluster (after grok #896 and gemini
#898). Wraps 'codex exec -s read-only --skip-git-repo-check' (default)
or 'codex review' (with --review). --model is ignored in --review
mode with a warning to stderr.

Bakes in all the round-2 + round-3 fixes from the gemini.ts review
cycle:
- exit codes 0/1/2 uniform per tools/peer-call/README.md (NOT 64)
- commandAvailable via /bin/sh -c 'command -v <cmd>' (PATH check, not
  --version)
- /bin/bash -c (not /bin/sh -c) for context-cmd to preserve bash-only
  feature support (Codex P2)
- ReadHeadResult { ok, content, error } so file-read failures surface
  to stderr instead of silently dropping context
- classifySpawnFailure 4-case helper (status / ENOENT → 127 / signal /
  other) reused from PR #887 + slice-16 #898
- Concat stdout + stderr from runContextCmd to preserve shell parse
  errors that fall outside the inner ( ) 2>&1 redirect

Lint-clean: bun --bun tsc --noEmit + eslint strictTypeChecked +
sonarjs all pass. Argument-validation byte-equivalent.

Note: same CodeQL js/indirect-command-line-injection alert will fire
here as on grok.ts and gemini.ts. Per B-0107 (filed in PR #897) the
per-PR dismissal pattern applies until B-0107 implements the
structural fix.
AceHack added a commit that referenced this pull request Apr 30, 2026
…ull-stream guards + header phrasing

Three Copilot findings on #901:

P0 — spawnSync launch failures collapsed into exitCode 1:
  Added classifySpawnFailure helper (4-case: status set / ENOENT
  → 127 / signal / other) reused from PRs #887, #898, #900. Both
  runSnapshotBurn and runProjectRunway now report a contextual
  note when the child can't start (e.g., 'snapshot-burn.sh:
  command not found on PATH (ENOENT)').

P0 — null stdout/stderr could yield 'nullnull':
  When a child fails to start, result.stdout / result.stderr
  can be null even with encoding: 'utf8'. Guarded with `?? ''`
  in runProjectRunway so the projection block doesn't end up as
  the literal string 'nullnull'.

P2 — Header comment phrasing:
  Reworded 'snapshot-burn.sh ported in #894' to 'TS port
  snapshot-burn.ts landed in #894 but this wrapper still spawns
  the .sh during the soak period' to avoid implying the .sh file
  itself is the ported artifact.
AceHack added a commit that referenced this pull request Apr 30, 2026
…tion (#901)

* ts(B-0086): port 1 budget script (.sh→.ts) — slice 18 of TS/Bun migration

* ts(slice-18, wip 1/N): port budget/daily-cost-report (.sh→.ts)

Daily cost-monitoring entry point. Wraps snapshot-burn.sh +
project-runway.sh and writes human-readable projection to
docs/budget-history/latest-report.md (visibility surface for
Aaron's #287 deadline).

Note: this wrapper still spawns the bash siblings (snapshot-burn.sh
+ project-runway.sh), NOT the TS port — the bash versions are the
soak-period reference until they retire. Once project-runway is
also TS-ported, this wrapper can switch to spawning the .ts versions.

Mechanical changes:
- bash arg-parse → parseArgs with ParsedArgs | ArgError | help
- bash 'cat > "$report_path" <<EOF...EOF' heredoc → buildReport()
  template literal returning the markdown string
- bash subshell command capture ($(...)) → spawnSync with stdio
  ['inherit', 'pipe', 'pipe'] for project-runway combined output
- bash heredoc concat across multi-line → resolved via separate
  argsSuffix variable (sonarjs no-nested-template-literals)
- exit codes 0/1/2 preserved verbatim per bash original

Lint-clean: tsc --noEmit + eslint strictTypeChecked + sonarjs all
pass. Argument-validation byte-equivalent.

Trajectory: 39 ports total after merge, 4 Bucket B files remain
(1 budget project-runway + 1 git/batch-resolve + 1 pr-preservation
+ 1 in-flight = 4 remaining).

* review(slice-18): use statSync.isFile() instead of existsSync per Codex P2

Codex P2: existsSync returns true for directories and other
non-regular paths; the bash original uses -f which checks
regular-file existence. If snapshots.jsonl were ever a directory,
existsSync would skip the bootstrap branch and the wrapper would
try to spawn project-runway.sh against a non-file. Switched to
statSync.isFile() with try/catch fallback to false.

* review(slice-18): address Copilot P0+P0+P2 — spawn classification + null-stream guards + header phrasing

Three Copilot findings on #901:

P0 — spawnSync launch failures collapsed into exitCode 1:
  Added classifySpawnFailure helper (4-case: status set / ENOENT
  → 127 / signal / other) reused from PRs #887, #898, #900. Both
  runSnapshotBurn and runProjectRunway now report a contextual
  note when the child can't start (e.g., 'snapshot-burn.sh:
  command not found on PATH (ENOENT)').

P0 — null stdout/stderr could yield 'nullnull':
  When a child fails to start, result.stdout / result.stderr
  can be null even with encoding: 'utf8'. Guarded with `?? ''`
  in runProjectRunway so the projection block doesn't end up as
  the literal string 'nullnull'.

P2 — Header comment phrasing:
  Reworded 'snapshot-burn.sh ported in #894' to 'TS port
  snapshot-burn.ts landed in #894 but this wrapper still spawns
  the .sh during the soak period' to avoid implying the .sh file
  itself is the ported artifact.

* review(slice-18) round-2: extract step-helpers + suppress no-unnecessary-condition

- Extracted runSnapshotStep + runProjectionStep helpers to drop
  main() under cognitive-complexity 15.
- Added eslint-disable on stdout/stderr ?? '' guards (typings claim
  string when encoding is set, but the runtime can return null when
  spawn fails — same pattern as #898).

Lint clean: tsc + eslint strictTypeChecked + sonarjs all pass.

* review(slice-18): preserve stdout/stderr ordering via shell-side 2>&1 per Copilot P1

Copilot caught: concatenating result.stdout + result.stderr does NOT
preserve the original chronological ordering of merged streams.
The bash original $(... 2>&1) merges at the kernel pipe level — if
project-runway.sh emits warnings on stderr while writing success
output to stdout, the messages interleave correctly.

Switched to /bin/bash -c 'path 2>&1' so the merge happens shell-side
(matches bash original semantics). Single stdout pipe = correct ordering.
result.stderr is now unused (the inherit pipe still receives nothing).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants